Update the sort logic in the assets grouping #15815

MikeAlhayek · 2024-04-23T23:30:15Z

No description provided.

coderabbitai · 2024-04-23T23:30:53Z

Walkthrough

The recent updates across various scripts and stylesheets primarily focus on code optimization, refactoring, and minor functionality adjustments. Changes include the introduction of new functions for improved class inheritance, the removal of redundant lines and comments, and enhancements in user interaction elements like hover behaviors. These modifications aim to enhance maintainability, readability, and performance of the codebase.

Changes

File Path	Change Summary
`gulpfile.js`	Updated asset group path resolution with wildcard support and consistent concatenation order.
`.../admin-menu-icon-picker.js`, `.../admin-menu.js`, `.../menu.js`, `.../Taxonomies/wwwroot/Scripts/menu.js`	Removed lines related to asynchronous execution and data object removal; improved hover interactions.
`.../diff.js`, `.../popper.js`, `jquery.js`, `jquery.slim.js`	Minor changes, mainly removal of empty lines, affecting only formatting.
`.../bootstrap.bundle.js`, `.../bootstrap.js`	Refactored code structure; introduced `_callSuper`, modified `_isNativeReflectConstruct`, and updated utility functions for better class handling.
`.../codemirror.js`, `.../trumbowyg-plugins.js`	Elimination of unnecessary line breaks and redundant comments.
`.../trumbowyg.js`	Modified handling of the `strikethrough` command when `semantic` option is enabled.
`.../setup.js`	Code refactoring: removed redundant code, reordered functions, and refined implementations for clarity and performance.
`.../TheAdmin/wwwroot/css/TheAdmin.css`	Corrected a comment related to CSS imports.
`.../TheAdmin/wwwroot/js/TheAdmin.js`	Removed unnecessary comments and empty lines; updated symbol handling and string conversions in utility functions.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

MikeAlhayek · 2024-04-23T23:43:23Z

@Piedone why would the validation the building fails? I rebuild the assets.

Piedone · 2024-04-23T23:49:04Z

What's exactly the goal here?

Check out the workflow output with the diff, and/or download the files the workflow built under https://github.com/OrchardCMS/OrchardCore/actions/runs/8808647832?pr=15815 and compare them locally.

MikeAlhayek · 2024-04-24T00:01:59Z

@Piedone the goal is as follow

// The inputPaths can contain either a physical path to a file or a path with a wildcard.
// It's crucial to maintain the order of each file based on its position in the assets.json file.
// When a path contains a wildcard, we need to convert the wildcard to physical paths
// and sort them independently of the previous paths to ensure consistent concatenation.

so if we have

[
  {
    "inputs": [
      "./physical-path1.js",
      "./Assets/js/*.js",
      "./physical-path2.js",
    ]
  }
]

we should append the files in this order

physical-path1.js
all the files sorted from the js folder.
physical-path2.js

MikeAlhayek · 2024-04-24T00:03:41Z

Check out the workflow output with the diff, and/or download the files the workflow built under https://github.com/OrchardCMS/OrchardCore/actions/runs/8808647832?pr=15815 and compare them locally.

yes but it should not fail here since I rebuild the assets.

Piedone · 2024-04-24T00:19:41Z

Do we have a use case for this among our assets?

What I'd do is download the files and check out the diffs locally. That'll hint at what may go differently.

hishamco · 2024-04-24T01:04:15Z

@MikeAlhayek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 8 seconds before requesting another review.

Seems a lot of us will wait if there's a rate limiting :)

coderabbitai

Actionable comments posted: 0

Out of diff range and nitpick comments (1)

src/OrchardCore.Modules/OrchardCore.Resources/wwwroot/Scripts/codemirror/codemirror.js (1)
Line range hint 1-6: Consider logging exceptions in the empty catch block to avoid suppressing potential errors.
- } catch (e) {}
+ } catch (e) {
+   console.error("Error encountered:", e);
+ }

github-actions · 2024-04-25T20:39:42Z

This pull request has merge conflicts. Please resolve those before requesting a review.

MikeAlhayek · 2024-05-02T17:19:21Z

@Piedone I still don't understand why the WF is failing. The order of the concatenated files could have been changed. but the fact that I build the assets should be enough for the WF to work. Are you able to look into it little more please? I think the order of files in this PR is correct and should work.

Piedone · 2024-05-02T17:24:45Z

Did you download the workflow artifact and check for the changes locally, compared to your files?

MikeAlhayek · 2024-05-02T20:53:30Z

@Piedone you should be able to see what changed using the diff view here. Looks like spacing changed and order of there the code is added. But, since I recreated all of the resources, it should not detect anything that isn't generated.

This looks strange, it is changing the order of the concatenated files, but it seems that the method definition is changing. I am working if npm install is using a different package version for example "gulp-postcss": "^9.0.1" is in the dependency, when we run npm install its going to install gulp-postcss 9.0.latest and use that.

MikeAlhayek · 2024-05-02T20:56:20Z

Here is another strange one. the new files are generated with a different parameter name for the function than the one in production.

Piedone · 2024-05-02T22:42:02Z

Running npm install, then npm run rebuild for me yields a bunch of changes in this branch too, e.g.:

Downloading the artifact from https://github.com/OrchardCMS/OrchardCore/actions/runs/8930280187?pr=15815 yields slightly different changes.

So, it seems that the Gulpfile changes cause the merged files to be flaky/dependent on the platform. I don't know why.

However, I don't think it's worth putting any more time in this. I've spent about a workday sorting this out previously, with the goal of getting it to a stable state, so we can keep updating assets and get a reliable feedback if we've forgotten that. The changes in this PR address a use case that we don't actually have. It'd be nice to have (but still only "nice to have" and low on the list of priorities in my book due to the lack of use cases) if we were to want to maintain this assets pipeline for the foreseeable future, but we want to get rid of it.

MikeAlhayek · 2024-05-02T23:28:41Z

@Piedone We actually use it and should fix it.

Here is one example
https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Modules/OrchardCore.Media/Assets.json#L11-L14

Piedone · 2024-05-02T23:40:07Z

I see. I searched and that's the only file where globbing and static paths are mixed. Only globbing is supported, even if multiple wildcard paths are listed (what also happens a few times). So, I suggest expanding that config manually by listing all the files, or converting it to an all-wildcard one.

And if you want to really diligent, then also add a validation here that will fail the pipeline if the two are mixed.

MikeAlhayek · 2024-05-03T00:00:41Z

Here is the output of that media. The results are what I am expecting.

The logic in main is wrong and the use of .sort() is incorrect in main

[
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/node_modules/blueimp-file-upload/js/jquery.fileupload.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/node_modules/blueimp-file-upload/js/jquery.iframe-transport.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/helpers.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaApp/app.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaApp/fileupload.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaApp/folderComponent.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaApp/font-awesome-thumbnails.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaApp/mediaItemsGridComponent.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaApp/mediaItemsTableComponent.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaApp/pagerComponent.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaApp/sortIndicatorComponent.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaField/attachedMediaField.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaField/mediaFieldThumbsContainer.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaField/mediafield.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/MediaField/mediafieldsAppsArray.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/Shared/uploadComponent.js',
  'C:/Code/OrchardCMS/OrchardCore/src/OrchardCore.Modules/OrchardCore.Media/Assets/js/app/Shared/uploadListComponent.js'
]

MikeAlhayek · 2024-05-03T14:08:31Z

@Piedone your soon to be favor PR is passing. I am wondering if there was something wrong with my environment. But what I did was to run npm remove gulp-print gulp-debug which generated the updated packages-lock.json file. New files were generated locally and it passed. I am not sure if all I needed was npm cache clean --force

I am just happy that it finally worked and the files sorting is correct.

MikeAlhayek · 2024-05-06T16:59:54Z

@Piedone you good with this?

Piedone

I wasn't aware that you're requesting my review here, sorry.

Does gulp watch work for you? For me, it gets stuck on this, never completing and thus never changing the output files:

MikeAlhayek · 2024-05-09T14:53:26Z

@Piedone yes it works for me. Here is what I get as I am saving random file that are being watched

Piedone · 2024-05-09T14:59:15Z

That seems like an in-progress build still. Does the output file change in the end?

MikeAlhayek · 2024-05-09T15:05:54Z

It does change for me.

Piedone

OK then!

MikeAlhayek added 2 commits April 23, 2024 16:28

Update the wildcard sort logic in gulpfile.

bfa3f9d

Rebuild Assets

dc0ce32

MikeAlhayek requested a review from Piedone April 23, 2024 23:30

Merge branch 'main' into ma/fix-gulpfile

f04a12f

simplify

b0cf98c

coderabbitai bot reviewed Apr 24, 2024

View reviewed changes

github-actions bot added the merge conflict label Apr 25, 2024

MikeAlhayek added 2 commits April 28, 2024 11:03

rebuild

324e79d

Merge branch 'main' into ma/fix-gulpfile

78800b9

github-actions bot removed the merge conflict label Apr 28, 2024

Rebuild after merge-conflict

7817cdc

This was referenced May 1, 2024

Monthly community metrics report for 2024-04-01..2024-04-30 #15929

Closed

Monthly community metrics report for 2024-04-01..2024-04-30 Emmanjr/OrchardCore#2

Open

Merge branch 'main' into ma/fix-gulpfile

926db3c

update the sort logic

735f244

MikeAlhayek added 5 commits May 3, 2024 06:15

use nocase

c6fd51f

nocase false

c511b79

remove options

499435d

Merge branch 'main' into ma/fix-gulpfile

42455e8

new files!!

a4322df

Piedone reviewed May 6, 2024

View reviewed changes

Piedone approved these changes May 9, 2024

View reviewed changes

MikeAlhayek merged commit 1ba8423 into main May 9, 2024
16 checks passed

MikeAlhayek deleted the ma/fix-gulpfile branch May 9, 2024 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the sort logic in the assets grouping #15815

Update the sort logic in the assets grouping #15815

MikeAlhayek commented Apr 23, 2024

coderabbitai bot commented Apr 23, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

MikeAlhayek commented Apr 23, 2024

Piedone commented Apr 23, 2024

MikeAlhayek commented Apr 24, 2024 •

edited

Loading

MikeAlhayek commented Apr 24, 2024

Piedone commented Apr 24, 2024

hishamco commented Apr 24, 2024

coderabbitai bot left a comment

github-actions bot commented Apr 25, 2024

MikeAlhayek commented May 2, 2024

Piedone commented May 2, 2024

MikeAlhayek commented May 2, 2024

MikeAlhayek commented May 2, 2024

Piedone commented May 2, 2024

MikeAlhayek commented May 2, 2024

Piedone commented May 2, 2024

MikeAlhayek commented May 3, 2024

MikeAlhayek commented May 3, 2024

MikeAlhayek commented May 6, 2024

Piedone left a comment

MikeAlhayek commented May 9, 2024

Piedone commented May 9, 2024

MikeAlhayek commented May 9, 2024

Piedone left a comment

Update the sort logic in the assets grouping #15815

Update the sort logic in the assets grouping #15815

Conversation

MikeAlhayek commented Apr 23, 2024

coderabbitai bot commented Apr 23, 2024 • edited Loading

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

MikeAlhayek commented Apr 23, 2024

Piedone commented Apr 23, 2024

MikeAlhayek commented Apr 24, 2024 • edited Loading

MikeAlhayek commented Apr 24, 2024

Piedone commented Apr 24, 2024

hishamco commented Apr 24, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 25, 2024

MikeAlhayek commented May 2, 2024

Piedone commented May 2, 2024

MikeAlhayek commented May 2, 2024

MikeAlhayek commented May 2, 2024

Piedone commented May 2, 2024

MikeAlhayek commented May 2, 2024

Piedone commented May 2, 2024

MikeAlhayek commented May 3, 2024

MikeAlhayek commented May 3, 2024

MikeAlhayek commented May 6, 2024

Piedone left a comment

Choose a reason for hiding this comment

MikeAlhayek commented May 9, 2024

Piedone commented May 9, 2024

MikeAlhayek commented May 9, 2024

Piedone left a comment

Choose a reason for hiding this comment

coderabbitai bot commented Apr 23, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

MikeAlhayek commented Apr 24, 2024 •

edited

Loading